-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrated code lifecycle
: Configure checkout path and timeout for programming exercises
#9217
base: develop
Are you sure you want to change the base?
Integrated code lifecycle
: Configure checkout path and timeout for programming exercises
#9217
Conversation
Integrated code lifecycle
: Configure checkout path and timeout for programming exercises
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Line range hint
291-309
: Ensure consistent null and blank checks for checkout paths.The conditions for setting
assignmentCheckoutPath
,testCheckoutPath
, andsolutionCheckoutPath
differ slightly. For consistency and to avoid potential null pointer exceptions, ensure all conditions uniformly check forisBlank()
:// Assignment checkout path assignmentCheckoutPath = (!StringUtils.isBlank(assignmentCheckoutPath)) ? assignmentCheckoutPath : RepositoryCheckoutPath.ASSIGNMENT.forProgrammingLanguage(programmingLanguage); // Test checkout path - String defaultTestCheckoutPath = RepositoryCheckoutPath.TEST.forProgrammingLanguage(programmingLanguage); - testCheckoutPath = (!StringUtils.isBlank(defaultTestCheckoutPath) && !StringUtils.isBlank(testCheckoutPath)) ? testCheckoutPath : defaultTestCheckoutPath; + testCheckoutPath = (!StringUtils.isBlank(testCheckoutPath)) ? testCheckoutPath : RepositoryCheckoutPath.TEST.forProgrammingLanguage(programmingLanguage); // Solution checkout path solutionCheckoutPath = (!StringUtils.isBlank(solutionCheckoutPath)) ? solutionCheckoutPath : RepositoryCheckoutPath.SOLUTION.forProgrammingLanguage(programmingLanguage);This ensures that all checkout paths are set using the same logic, enhancing code readability and reducing confusion.
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/main/resources/templates/javascript/test/package-lock.json
is excluded by!**/package-lock.json
Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (6 hunks)
- src/main/resources/templates/javascript/test/.gitignore (1 hunks)
- src/main/resources/templates/javascript/test/package.json (1 hunks)
Additional context used
Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Additional comments not posted (2)
src/main/resources/templates/javascript/test/.gitignore (1)
3-3
: LGTM!The change from a hardcoded directory name to a dynamic reference using
${studentParentWorkingDirectoryName}
aligns with the PR objective of allowing customizable checkout paths for programming exercises. It enhances the flexibility of the build job setup without introducing any apparent issues.src/main/resources/templates/javascript/test/package.json (1)
9-9
: Flexible workspace configuration introduced.The change from a static "assignment" directory to a dynamic reference
${studentParentWorkingDirectoryName}
for the "workspaces" property introduces flexibility in configuring the workspace directory. This can be beneficial for accommodating different project structures or configurations.Please ensure that the variable
${studentParentWorkingDirectoryName}
is properly defined and resolved to a valid directory name to avoid potential issues. Additionally, verify compatibility with existing projects or configurations that may rely on the static "assignment" directory.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (5 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java (6 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2 hunks)
Additional context used
Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Additional comments not posted (8)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2)
667-691
: Enhancements to placeholder handling look good!The changes in this method provide more flexibility in configuring the working directories for different repository types. Key improvements include:
- Using the build config to override default paths
- Special handling for Python to replace "/" with "." in the student working directory
- Additional placeholder for Java with Gradle and Rust to support their directory structures
Overall, these enhancements align with the summary and improve the placeholder replacement logic.
Line range hint
692-789
: Access control logic based on exercise configuration changes is implemented correctly.This method plays a crucial role in maintaining the integrity of the programming exercise by ensuring that the repository and participation access rights are updated based on the changes in the exercise configuration.
It correctly handles various scenarios such as:
- Locking access when the start date or due date becomes stricter or offline IDE usage is disallowed.
- Unlocking access when the start date or due date becomes more lenient or offline IDE usage is allowed.
The implementation follows a clear and comprehensive logic to cover all possible combinations of configuration changes.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (6)
27-27
: Updated import to Apache Commons Lang 3StringUtils
The import statement has been correctly updated to use
org.apache.commons.lang3.StringUtils
, ensuring compatibility with Apache Commons Lang 3.
279-284
: Added JavaDoc for new parametersThe JavaDoc now includes the new parameters
assignmentCheckoutPath
,testCheckoutPath
, andsolutionCheckoutPath
, with clear descriptions of their purpose.
287-289
: Method signature updated with new parametersThe
populateBuildJobContainer
method now acceptsassignmentCheckoutPath
,testCheckoutPath
, andsolutionCheckoutPath
parameters, enhancing configurability.
291-292
: Correct usage ofStringUtils.isBlank
forassignmentCheckoutPath
The logic correctly assigns the default path if
assignmentCheckoutPath
is blank, ensuring that the default path is used when no custom path is provided.
307-308
: Consistent handling ofsolutionCheckoutPath
The assignment of
solutionCheckoutPath
is consistent withassignmentCheckoutPath
, correctly using the default path whensolutionCheckoutPath
is blank.
446-449
: RefactoredgetParentFolderPath
usingPath
APIThe
getParentFolderPath
method now utilizesPaths.get()
,normalize()
, andgetParent()
for OS-independent path handling and improved readability.
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-approve after JS template was added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove code
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on Ts2, Ocaml works now by adjusting the build script, same for Assembler 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on ts2, works as expected
…checkout-path-and-timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (4 hunks)
- src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (2 hunks)
- src/main/webapp/i18n/de/programmingExercise.json (1 hunks)
- src/main/webapp/i18n/en/programmingExercise.json (1 hunks)
Additional context used
Path-based instructions (3)
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)
src/main/webapp/i18n/de/programmingExercise.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json
: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
Additional comments not posted (11)
src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (5)
55-56
: LGTM: Improved Docker image field retrieval.The use of the nullish coalescing operator (
??
) to retrieve thedockerImageField
from either component is a good practice. It ensures robust handling of potentially undefined values while maintaining clean code.
61-66
: LGTM: Enhanced timeout field handling.The addition of
timeoutField
retrieval and subscription aligns well with the PR objectives. The code structure is consistent with the existingdockerImageField
handling, which is good for maintainability.Note on
setTimeout
usage:
The use ofsetTimeout
in the subscription callback is consistent with the existing pattern in the file. While this approach works, consider exploring Angular-specific alternatives likeChangeDetectorRef.detectChanges()
orNgZone.run()
for potentially better integration with Angular's change detection mechanism.
98-101
: LGTM: Improved validation for PROFILE_LOCALCI.The enhanced validation logic now correctly checks both
dockerImageField
andtimeoutField
validity. The use of optional chaining and nullish coalescing operators ensures robust handling of potentially undefined values. This change aligns well with the PR objectives to improve the configurability of programming exercise build processes.
105-108
: LGTM: Consistent validation improvement for PROFILE_AEOLUS.The validation logic for
PROFILE_AEOLUS
has been updated consistently with the changes made forPROFILE_LOCALCI
. This consistency in approach across different profiles is commendable and contributes to the overall maintainability of the code. The enhanced validation aligns well with the PR objectives, ensuring both Docker image and timeout configurations are properly validated.
Line range hint
1-114
: Overall assessment: Well-implemented enhancements to programming exercise configuration.The changes in this file consistently improve the validation logic for Docker image and timeout configurations across different profiles (
PROFILE_LOCALCI
andPROFILE_AEOLUS
). The code demonstrates good practices such as:
- Consistent use of optional chaining and nullish coalescing operators for robust error handling.
- Adherence to coding guidelines, including naming conventions and code style.
- Logical enhancements that align well with the PR objectives.
The implementation maintains a good balance between functionality enhancement and code maintainability. The consistent approach across different profiles is particularly commendable.
Minor suggestion for future consideration:
While the current implementation works well, consider exploring Angular-specific alternatives tosetTimeout
in reactive contexts, such asChangeDetectorRef.detectChanges()
orNgZone.run()
, for potentially better integration with Angular's change detection mechanism.Overall, these changes significantly contribute to the improved configurability of programming exercise build processes as intended by the PR objectives.
src/main/webapp/i18n/en/programmingExercise.json (2)
143-147
: LGTM! Clear and concise timeout information.The "timeout" section provides clear and concise information about customizing the build timeout. The note about administrative limits potentially overriding the user-set value is an important detail for users to be aware of.
131-147
: Excellent additions enhancing exercise customization options.The new "checkoutPath" and "timeout" sections are well-integrated into the existing structure of the file. They provide clear, concise, and valuable information to users about customizing repository checkout paths and build timeouts. The warnings and alerts help users understand the implications of their choices, which is crucial for preventing potential issues.
These additions significantly enhance the flexibility and user control over programming exercises while maintaining consistency with the rest of the localization file.
src/main/webapp/i18n/de/programmingExercise.json (1)
118-129
: Gut gemachte Übersetzungen für "checkoutPath"!Die Übersetzungen sind korrekt und verwenden durchgehend die informelle Anrede ("du"-Form), was den Vorgaben entspricht. Die Texte sind klar und informativ, und die JSON-Struktur ist einwandfrei.
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (3)
414-414
: Properly determine edit mode withisEdit
flagThe addition of
this.isEdit = segments.some((segment) => segment.path === 'edit');
correctly identifies whether the component is in edit mode based on the route parameters.
879-880
: Include timeout validation in invalid reasons checkAdding
this.validateTimeout(validationErrorReasons);
ensures that the timeout value is properly validated and any issues are reported to the user.
1115-1119
: FunctionareValuesUnique
correctly checks for uniquenessThe function effectively filters out undefined and empty strings and checks for duplicates among checkout paths.
...main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts
Show resolved
Hide resolved
...main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts
Show resolved
Hide resolved
...main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (4 hunks)
- src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (2 hunks)
- src/main/webapp/i18n/de/programmingExercise.json (1 hunks)
- src/main/webapp/i18n/en/programmingExercise.json (1 hunks)
Additional context used
Path-based instructions (3)
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)
src/main/webapp/i18n/de/programmingExercise.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json
: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
Additional comments not posted (3)
src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)
55-56
: LGTM: Proper handling of potential undefined values.The use of the nullish coalescing operator (??) to retrieve the
dockerImageField
from either component is a good practice. It ensures that the code gracefully handles cases where one of the components might be undefined.src/main/webapp/i18n/en/programmingExercise.json (1)
Line range hint
1-147
: Overall changes look good and enhance user experience.The additions to the localization file for "checkoutPath" and "timeout" are well-structured, informative, and consistent with the existing content. These new entries will help users understand and utilize the new features effectively.
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
414-414
: LGTMThe addition of
isEdit
correctly sets the edit mode based on the route parameters.
Contains migration
Checklist
General
Server
Client
Changes affecting Programming Exercises
Motivation and Context
Second PR of this feature proposal: #8887
We want the instructors to be able to customize the build timeout and the checkout paths
Steps for Testing
Quick testing (On test servers)
Prerequisites:
Checkout Paths testing
Customize Build Script
, If the build script uses the default checkout paths (assignment/solution/tests) make sure to change them accordinglyTimeout testing
Testing Locally (Optional)
Prerequisites:
.withAutoRemove(false);
docker exec -it <mycontainer> sh
Navigate using the cliTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor